Support RAID configuration for baremetal server by longkb · Pull Request #292 · metal3-io/baremetal-operator

longkb · 2019-08-28T13:48:58Z

Support RAID configuration for baremetal server

Currently, Metal3 does not support deploy baremetal server with RAID configuration (#206).
This PR aims to extend Ironic Provisioner to support RAID configuration with the bellows:

Extend BaremetalHost CRD to support RAID configuration
Generate Ironic's cleansteps from raid property in host's spec.
Trigger manual cleaning to config raid in provisioning phase.

nordixinfra · 2019-08-28T13:51:06Z

Can one of the admins verify this patch?

dhellmann

This version is looking pretty good.

I think we can improve the validation by using kubebuilder features. I have tried to give an example in an inline comment.

I would like for us to add the smallest possible API for managing RAID. It is easier to add to the API than to remove from it once it is being used. If there are reasonable default behaviors, we should leave options out, for now. If there are advanced features, we may never want to include them.

I think the code to start the manual cleaning process needs to be run in more places. I have tried to explain in more detail inline.

dhellmann · 2019-08-30T20:06:36Z

+
+	// Integer, number of disks to use for the logical disk. Defaults to minimum number of disks required
+	// for the particular RAID level.
+	NumberOfPhysicalDisks int `json:"numberOfPhysicalDisks,omitempty"`


When would you ever set this to anything other than the default for a given RAID mode?

I never try to set this before.

Well, you can have non-default number of disks, especially for RAID-0.

How does the validation instruction work with this field? If the user doesn't specify a value, will we get 0? But if they do specify a value then the validation ensures that it is > 0?

Opps, I missed that point.
In the case of user didn't specify a value, we will pass the validation of kubebuilder and got 0.
And if they specify a value, it will be validate by kubebuilder in line 129 // +kubebuilder:validation:Minimum=1
So I think pointer would be suitable in the case of missing this field
Thanks

Unresolved this because it appears we decided on a pointer here but the field didn't actually change.

Also for software RAID we're supposed to be able to supply device hints in the same format as the root device hints we added in #495, and the DiskType and NumberOfPhysicalDisks do not apply. If we're going to keep the software RAID thing I wonder if we should separate the software and hardware RAID configurations into separate structures.

How about this:

// RAIDConfig contains the configuration that are required to config RAID in Bare Metal server type RAIDConfig struct { // If true will remove existing raid configuration and set new raid configuration. DeleteExisting bool `json:"deleteExisting,omitempty"` // The list of logical disks for hardware RAID, first volume is root volume HVolumes []HRAIDVolume `json:"hVolumes,omitempty"` // The list of logical disks for software RAID, first volume is root volume, if HVolume is setted this items will be invalid. // The number of created Software RAID devices must be 1 or 2. // If there is only one Software RAID device, it has to be a RAID-1. // If there are two, the first one has to be a RAID-1, while the RAID level for the second one can 0, 1, or 1+0. // As the first RAID device will be the deployment device, // enforcing a RAID-1 reduces the risk of ending up with a non-booting node in case of a disk failure. SVolumes []SRAIDVolume `json:"sVolumes,omitempty"` }

longkb · 2019-09-04T11:13:07Z

@dhellmann , @dtantsur : Thank for your comments
Sorry for late reply. I just push the revision to address your comments :)

dhellmann · 2019-09-18T17:00:46Z

+
+	// Integer, number of disks to use for the logical disk. Defaults to minimum number of disks required
+	// for the particular RAID level.
+	NumberOfPhysicalDisks int `json:"numberOfPhysicalDisks,omitempty"`


longkb

@dhellmann Thanks for you reviewing. I have pushed some revision in this PR :)

dhellmann · 2020-01-09T17:40:32Z

+
+	// The valid values must be "hdd" or "ssd". If this is not specified, disk type will not be a criterion to find backing physical disks
+	// +kubebuilder:validation:Enum=hdd,ssd
+	DiskType nodes.DiskType `json:"diskType,omitempty"`


If this field is optional, don't we need for it to be a pointer? Or does DiskType have a good zero value?

Although Ironic will handle the zero value for DiskType, but I think it would be better if we make it to be a pointer
Thanks

This still needs to be addressed. What is the zero value for nodes.DiskType? What default do we want for this field?

I have no idea, but in OpenStack Docs, a part of example haven't set disk_type, and i find some explanation.

disk_type - hdd or ssd. If this is not specified, disk type will not be a criterion to find backing physical disks.

Or we can check DiskType, if DiskType is empty return error.

It's just a string so the zero value is an empty string 🤷‍♂️

We probably do have to specifically allow an empty value in the kubebuilder validation of the previous line though I think?

Everywhere else in the API (HardwareDetails and root device hints) we have had a bool named Rotational instead of the strings "hdd" and "sdd". Let's do that here also and make this a bool pointer.

dhellmann · 2020-01-09T17:41:16Z

+
+	// Integer, number of disks to use for the logical disk. Defaults to minimum number of disks required
+	// for the particular RAID level.
+	NumberOfPhysicalDisks int `json:"numberOfPhysicalDisks,omitempty"`


How does the validation instruction work with this field? If the user doesn't specify a value, will we get 0? But if they do specify a value then the validation ensures that it is > 0?

dtantsur

I don't think I can lgtm here, but looks good

dtantsur · 2021-02-24T10:34:22Z

 }

+// HardwareRAIDVolume defines the desired configuration of volume in hardware RAID
+type HardwareRAIDVolume struct {


nit: we should probably move this to GopherCloud eventually

dtantsur · 2021-02-24T10:34:38Z

+	SizeGibibytes *int `json:"sizeGibibytes,omitempty"`
+
+	// RAID level for the logical disk. The following levels are supported: 0;1;1+0.
+	// +kubebuilder:validation:Enum="0";"1";"1+0"


Yep (I'm fine with a follow-up)

dtantsur · 2021-02-24T10:35:28Z

+	SizeGibibytes *int `json:"sizeGibibytes,omitempty"`
+
+	// RAID level for the logical disk. The following levels are supported: 0;1;1+0.
+	// +kubebuilder:validation:Enum="0";"1";"1+0"


Let's probably open another patch and discuss there.

dtantsur · 2021-02-24T10:36:09Z

+				host.Status.Provisioning.RAID = &metal3v1alpha1.RAIDConfig{}
+				dirty = true
+			}
+			// If HardwareRAIDVolumes isn't nil, we will ignore SoftwareRAIDVolumes.


Let's research this option in a follow-up.

dtantsur · 2021-02-24T10:37:35Z

+}
+
+// A private method to build RAID disks
+func buildTargetRAIDCfg(raid *metal3v1alpha1.RAIDConfig) (logicalDisks []nodes.LogicalDisk, err error) {


nit: maybe make this public for reuse? e.g. we use these bits in openshift installer (if you care about it).

dtantsur · 2021-02-24T10:37:52Z

+}
+
+// buildRAIDCleanSteps build the clean steps for RAID configuration from BaremetalHost spec
+func buildRAIDCleanSteps(raid *metal3v1alpha1.RAIDConfig) (cleanSteps []nodes.CleanStep) {


Same re public

dtantsur · 2021-02-24T10:38:49Z

/test-integration

dtantsur · 2021-02-24T10:40:15Z

/hold cancel

Signed-off-by: zouyu <zouy.fnst@cn.fujitsu.com>

demonCoder95 · 2021-03-01T14:42:48Z

/test-integration

zouy414 · 2021-03-02T01:09:51Z

@andfasano @dtantsur @zaneb PTAL.

andfasano · 2021-03-02T11:55:13Z

+	if p.bmcAccess.RAIDInterface() != "" {
+		cleanSteps = append(cleanSteps, BuildRAIDCleanSteps(p.host.Status.Provisioning.RAID)...)
+	} else if p.host.Status.Provisioning.RAID != nil {
+		return nil, fmt.Errorf("RAID settings are exist, but the node's driver %s does not support RAID", p.bmcAccess.Driver())


typo: I guess it's RAID settings are defined

Thanks for your review, done.

andfasano · 2021-03-02T11:59:12Z

+	// Build manual clean steps
+	cleanSteps, err := p.buildManualCleaningSteps()
+	if err != nil {
+		result, err = transientError(err)


A transientError produces an immediate re-enqueue of the reconcile loop. IIUC since in this case the error generated by p.buildManualCleaningSteps() is synchronous (and could maybe require a human intervention to be fixed), I'd suggest to use operationFailed() since it will activate the retry mechanism with backoff

andfasano · 2021-03-02T11:59:32Z

+		var cleanSteps []nodes.CleanStep
+		cleanSteps, err = p.buildManualCleaningSteps()
+		if err != nil {
+			result, err = transientError(err)


andfasano · 2021-03-02T12:06:22Z

+						},
+					},
+				}
+			}


That's really a fragile approach, and moreover hides the setup in the cases list. In other situation we've used the builder pattern to allow each case to specify its own settings (as for the testserver), here's not yet available for the host, but at least an enrichment function can be added in the test struct, to be invoked on the host only when defined

Thanks, I added a flag existRaidConfig to test cases.

A good step in the right direction (at least more robust than before), even though the single test case readability is still not optimal (a builder approach would be definitely better); not anymore a blocking point for me though

Signed-off-by: zouyu <zouy.fnst@cn.fujitsu.com>

andfasano · 2021-03-03T12:23:44Z

+						},
+					},
+				}
+			}


A good step in the right direction (at least more robust than before), even though the single test case readability is still not optimal (a builder approach would be definitely better); not anymore a blocking point for me though

metal3-io-bot · 2021-03-03T12:26:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andfasano, dtantsur, longkb, zaneb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [zaneb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

zouy414 · 2021-03-04T01:49:02Z

@dhellmann @zaneb @andfasano and @dtantsur have approved this PR, is there anything else block the PR from being merged?

demonCoder95 · 2021-03-05T07:26:39Z

/test-integration

demonCoder95 · 2021-03-05T07:26:59Z

@andfasano @dtantsur I think this needs an LGTM to proceed.

dtantsur · 2021-03-08T14:42:10Z

/lgtm

I don't think I actually have LGTM rights on this repository, but let's try.

metal3-io-bot · 2021-03-08T14:42:13Z

@dtantsur: adding LGTM is restricted to approvers and reviewers in OWNERS files.

Details

In response to this:

/lgtm

I don't think I actually have LGTM rights on this repository, but let's try.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

rdoxenham · 2021-03-08T14:46:23Z

+                          maxLength: 64
+                          type: string
+                        numberOfPhysicalDisks:
+                          description: Integer, number of disks to use for the logical disk. Defaults to minimum number of disks required for the particular RAID level.


Suggested change

description: Integer, number of disks to use for the logical disk. Defaults to minimum number of disks required for the particular RAID level.

description: Integer, number of physical disks to use for the logical disk. Defaults to minimum number of disks required for the particular RAID level.

rdoxenham · 2021-03-08T14:48:12Z

+                              - 1+0
+                              type: string
+                            physicalDisks:
+                              description: A list of device hints, the number of item should be greater than or equal to 2.


Suggested change

description: A list of device hints, the number of item should be greater than or equal to 2.

description: A list of device hints, the number of items should be greater than or equal to 2.

rdoxenham · 2021-03-08T14:49:02Z

+                          - 1+0
+                          type: string
+                        physicalDisks:
+                          description: A list of device hints, the number of item should be greater than or equal to 2.


Suggested change

description: A list of device hints, the number of item should be greater than or equal to 2.

description: A list of device hints, the number of items should be greater than or equal to 2.

rdoxenham · 2021-03-08T14:50:18Z

+                              - 1+0
+                              type: string
+                            physicalDisks:
+                              description: A list of device hints, the number of item should be greater than or equal to 2.


Suggested change

description: A list of device hints, the number of item should be greater than or equal to 2.

description: A list of device hints, the number of items should be greater than or equal to 2.

rdoxenham · 2021-03-08T14:51:42Z

+                          - 1+0
+                          type: string
+                        physicalDisks:
+                          description: A list of device hints, the number of item should be greater than or equal to 2.


Suggested change

description: A list of device hints, the number of item should be greater than or equal to 2.

description: A list of device hints, the number of items should be greater than or equal to 2.

rdoxenham · 2021-03-08T14:52:45Z

+                              maxLength: 64
+                              type: string
+                            numberOfPhysicalDisks:
+                              description: Integer, number of disks to use for the logical disk. Defaults to minimum number of disks required for the particular RAID level.


Suggested change

description: Integer, number of disks to use for the logical disk. Defaults to minimum number of disks required for the particular RAID level.

description: Integer, number of physical disks to use for the logical disk. Defaults to minimum number of disks required for the particular RAID level.

rdoxenham · 2021-03-08T14:53:27Z

+                          maxLength: 64
+                          type: string
+                        numberOfPhysicalDisks:
+                          description: Integer, number of disks to use for the logical disk. Defaults to minimum number of disks required for the particular RAID level.


Suggested change

description: Integer, number of disks to use for the logical disk. Defaults to minimum number of disks required for the particular RAID level.

description: Integer, number of physical disks to use for the logical disk. Defaults to minimum number of disks required for the particular RAID level.

rdoxenham · 2021-03-08T14:53:59Z

+                              maxLength: 64
+                              type: string
+                            numberOfPhysicalDisks:
+                              description: Integer, number of disks to use for the logical disk. Defaults to minimum number of disks required for the particular RAID level.


Suggested change

description: Integer, number of disks to use for the logical disk. Defaults to minimum number of disks required for the particular RAID level.

description: Integer, number of physical disks to use for the logical disk. Defaults to minimum number of disks required for the particular RAID level.

andfasano · 2021-03-08T15:15:15Z

/lgtm

@demonCoder95 @Hellcatlk there are still a couple of points pending, but they could be addressed in a followup:

This comment from @dtantsur
The latest comments from @rdoxenham

zouy414 · 2021-03-09T08:24:44Z

@andfasano I submitted a PR to resolve the comments left by @rdoxenham.

longkb changed the title ~~Support raid configuration~~ Support RAID configuration for baremetal server Aug 28, 2019

longkb force-pushed the support_raid_configuration branch from 043f835 to 48ef146 Compare August 28, 2019 13:49

longkb mentioned this pull request Aug 28, 2019

Support RAID and BIOS configuration for Baremetal Server #279

Closed

dhellmann requested changes Aug 30, 2019

View reviewed changes

dtantsur reviewed Sep 2, 2019

View reviewed changes

Comment thread deploy/crds/metal3_v1alpha1_baremetalhost_crd.yaml Outdated

Comment thread pkg/apis/metal3/v1alpha1/baremetalhost_types.go Outdated

Comment thread pkg/provisioner/ironic/ironic.go Outdated

Comment thread pkg/provisioner/ironic/ironic.go Outdated

dhellmann requested changes Sep 3, 2019

View reviewed changes

Comment thread pkg/apis/metal3/v1alpha1/baremetalhost_types.go Outdated

Comment thread pkg/provisioner/ironic/ironic.go Outdated

Comment thread pkg/provisioner/ironic/ironic.go Outdated

longkb force-pushed the support_raid_configuration branch from 48ef146 to e41d52a Compare September 4, 2019 11:11

longkb force-pushed the support_raid_configuration branch 2 times, most recently from 3ab9a5e to d7a3d5a Compare September 9, 2019 10:15

dtantsur reviewed Sep 10, 2019

View reviewed changes

Comment thread pkg/provisioner/ironic/ironic.go Outdated

elfosardo requested changes Sep 10, 2019

View reviewed changes

Comment thread pkg/apis/metal3/v1alpha1/baremetalhost_types.go Outdated

longkb force-pushed the support_raid_configuration branch from d7a3d5a to 6f19d1c Compare September 11, 2019 10:04

dhellmann reviewed Sep 18, 2019

View reviewed changes

longkb force-pushed the support_raid_configuration branch from 6f19d1c to abd2434 Compare September 26, 2019 02:03

longkb commented Sep 26, 2019

View reviewed changes

longkb force-pushed the support_raid_configuration branch from abd2434 to 2368edb Compare October 4, 2019 02:26

metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 4, 2019

longkb commented Oct 4, 2019

View reviewed changes

Comment thread pkg/apis/metal3/v1alpha1/baremetalhost_types.go

longkb force-pushed the support_raid_configuration branch from 2368edb to 922ba8a Compare December 4, 2019 08:55

zaneb mentioned this pull request Dec 6, 2019

Configure RAID Interface for iRMC driver #319

Merged

This was referenced Dec 13, 2019

Cannot run OpenAPI code-generation for CRD with operator-sdk v0.12.0 #360

Closed

Adding code-generator for Kubernetes-style API #361

Closed

longkb force-pushed the support_raid_configuration branch from 922ba8a to 78b3314 Compare December 23, 2019 08:07

longkb force-pushed the support_raid_configuration branch from 78b3314 to f685941 Compare January 8, 2020 08:29

zaneb requested changes Jan 8, 2020

View reviewed changes

Comment thread pkg/apis/metal3/v1alpha1/baremetalhost_types.go Outdated

longkb force-pushed the support_raid_configuration branch from f685941 to d592398 Compare January 9, 2020 01:07

dhellmann reviewed Jan 9, 2020

View reviewed changes

longkb force-pushed the support_raid_configuration branch from d592398 to bd43382 Compare January 10, 2020 07:39

dtantsur approved these changes Feb 24, 2021

View reviewed changes

Add RAID Settings' API

5852424

Signed-off-by: zouyu <zouy.fnst@cn.fujitsu.com>

andfasano reviewed Mar 2, 2021

View reviewed changes

zouy414 added 2 commits March 3, 2021 09:21

Build RAID Config and clean steps

5399242

Signed-off-by: zouyu <zouy.fnst@cn.fujitsu.com>

Add unit tests

0745533

Signed-off-by: zouyu <zouy.fnst@cn.fujitsu.com>

andfasano approved these changes Mar 3, 2021

View reviewed changes

rdoxenham reviewed Mar 8, 2021

View reviewed changes

zouy414 mentioned this pull request Mar 9, 2021

Fix some typos in PR292 #809

Merged

demonCoder95 mentioned this pull request May 17, 2021

Extend BaremetalHost CRD to support BIOS configuration in baremetal server #302

Merged

MahnoorAsghar mentioned this pull request May 28, 2021

Airship RAID requirements #895

Closed

jezogwza mentioned this pull request Jul 14, 2021

Spike: Metal3.io Support for BIOS/Firmware Updates and RAID Configuration Changes airshipit/airshipctl#614

Closed

	description: Integer, number of disks to use for the logical disk. Defaults to minimum number of disks required for the particular RAID level.
	description: Integer, number of physical disks to use for the logical disk. Defaults to minimum number of disks required for the particular RAID level.

	description: A list of device hints, the number of item should be greater than or equal to 2.
	description: A list of device hints, the number of items should be greater than or equal to 2.

Conversation

longkb commented Aug 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nordixinfra commented Aug 28, 2019

Uh oh!

dhellmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zouy414 Jun 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

longkb commented Sep 4, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

longkb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zouy414 Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zouy414 Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

longkb commented Aug 28, 2019 •

edited

Loading

zouy414 Jun 19, 2020 •

edited

Loading

zouy414 Apr 28, 2020 •

edited

Loading

zouy414 Apr 28, 2020 •

edited

Loading